Feat #297 Support default values from rich contract data #349

rulfo71 · 2022-09-26T22:47:56Z

Description

Support default values from rich contract data. Disable input by default when receiving data. Adding unlock icon to enable.

22-09-26.Support.default.values.from.rich.contract.data.webm

Closes #297

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Tested manually and added Tests

Checklist:

My code follows the existing style of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any UI changes have been tested and made responsive for mobile views

…rom-rich-contract-data

github-actions · 2022-09-26T22:54:56Z

✔️ Preview deployment is ready!

🔨 Explore the source changes: 5245054

😎 Browse the preview: https://bafybeiazxxbgkz6kern2suj5zoiqzoa6wp72pyswxwbz2yazevtvujxgze.ipfs.cf-ipfs.com

github-actions · 2022-09-26T22:56:32Z

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeihifkqp7hm3cka773yp6ex6onwe4kxec6jfwig6mnhwxabzqlols4.ipfs.cf-ipfs.com

github-actions · 2022-09-27T16:04:21Z

✔️ Preview deployment is ready!

🔨 Explore the source changes: 3c6df13

😎 Browse the preview: https://bafybeigjndwkcqsosp6ksnzh2x2t67gtgwtlysbeczw5s2rifdvdbzu4ha.ipfs.cf-ipfs.com

github-actions · 2022-09-27T16:08:22Z

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeie4hrunndder2q7y47lb2ykj4nhinxu3qxxmbodwod4bakcctydva.ipfs.cf-ipfs.com

github-actions · 2022-09-27T16:16:28Z

✔️ Preview deployment is ready!

🔨 Explore the source changes: 0308aef

😎 Browse the preview: https://bafybeigjndwkcqsosp6ksnzh2x2t67gtgwtlysbeczw5s2rifdvdbzu4ha.ipfs.cf-ipfs.com

github-actions · 2022-09-27T16:16:48Z

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeigtv45hup6fegfqpz637y6vp5mcqiegsjqhs3gpmjmycjge2kuebm.ipfs.cf-ipfs.com

dcrescimbeni · 2022-09-27T17:14:23Z

Here's a video of the interactions.

This causes this side-effects in:

When we have a value in NumericalInput, it blocks its edit by default. Its OK in the context of editing default data from a contract, but unintended in other places we use that input, like a transfer call, or lock tokens.
A similar behaviour is shown while editing a SetPermission call: the address is locked and the user has to click the icon to unlock it and edit it.

If I understand the issue correctly, the locking logic (preventing a user to mistakenly modify a default value in a contract call) should be only applied while making a contract call, not in other places we use the same component.

What's tricky about this is that currently I think we don't seem to have a mechanism to tell the component that it is inside a contract call, and that's the main challenge.

https://www.loom.com/share/96abee75c4674e62b812e80f55c540ab

dcrescimbeni · 2022-09-27T13:26:21Z

src/components/primitives/Forms/NumericalInput.tsx


 const inputRegex = RegExp(`^\\d*(?:\\\\[.])?\\d*$`); // match escaped "." characters via in a non-capturing group

 export const NumericalInput: React.FC<InputProps<string>> = ({
  value,
  onChange,
  placeholder,
+  disabled = true,


This makes the default behaviour for all NumericalInputs to be locked (disabled).

The default should be disabled = false since in most cases its expected the user inputs a value.

This might be an unexpected side-effect of the fact that all values are initialized as BigNumber.from(0) t prevent errors.

Also it conflicts with the SetPermissions behaviour since a user can now unlock the amount field (originally locked and toggled to max).

dcrescimbeni · 2022-09-27T13:33:35Z

src/components/primitives/Forms/IconRight.tsx

+import { FiUnlock, FiX } from 'react-icons/fi';
+import { ClickableIcon } from './AddressInput';
+
+export const IconRight = ({
+  disabled = true,
+  value,
+  onChange,
+  setDisabled,
+  type,
+}) => {
+  const clearLabel = `clear ${type}`;
+
+  if (!disabled && value) {
+    return (
+      <ClickableIcon aria-label={clearLabel} onClick={() => onChange('')}>
+        <FiX size={18} />
+      </ClickableIcon>
+    );
+  } else if (disabled) {
+    return (
+      <ClickableIcon
+        aria-label="enable"
+        onClick={() => {
+          setDisabled(false);
+          console.log(disabled);
+        }}
+      >
+        <FiUnlock size={18} />
+      </ClickableIcon>
+    );
+  } else return null;
+};


Once we toggle the unlock button, we can't toggle it back on.

We have two competing icons: a lock and a clear (X) icon. Maybe if we show both the UI gets too crowded.

Just pointing this behaviour, I don't know what the end result should look like.

hmm, yes, this is true. i don't think the idea was to lock the input after unlocking it but it's good you point it out

I think we can leave the behaviour as is right now and later consider how we would have a lock icon

dcrescimbeni · 2022-09-27T13:41:50Z

src/components/primitives/Forms/AddressInput.tsx

+  const [disabledState, setDisabled] = useState(value ? disabled : false);
+  const iconRightProps = {
+    disabled: disabledState,
+    value: value,
+    onChange: onChange,
+    setDisabled,
+    type: 'address',
+  };


This locks the address field if it has value, like in the case of a user editing a function call permission.

…rom-rich-contract-data

rossneilson · 2022-09-30T15:16:27Z

src/components/primitives/Forms/NumericalInput.tsx


 const inputRegex = RegExp(`^\\d*(?:\\\\[.])?\\d*$`); // match escaped "." characters via in a non-capturing group

 export const NumericalInput: React.FC<InputProps<string>> = ({
  value,
  onChange,
  placeholder,
+  disabled = true,


I think passing defaultValue here would be more flexible in future and would limit the locking to when it is explicitly needed

rossneilson · 2022-09-30T15:17:55Z

We found a bug where action index 2 on the contract for voting in dxvote is showing index 1 option when clicked. We have not had this behaviour before so would be great to fix here if it is simple.

github-actions · 2022-10-03T13:17:57Z

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeidwarailizpqwpc2opbjcnkn4v6eipaxzspirvgcjplfejk7cfmpa.ipfs.cf-ipfs.com

rulfo71 · 2022-10-03T18:54:56Z

the branch is now mergeable, works locking the numerical or address input if it has a default value as requested.

Still missing:

tests
fix bug found where action index 2 on the contract for voting in dxvote is showing index 1 option when clicked

src/components/primitives/Forms/AddressInput.tsx

src/components/primitives/Forms/IconRight.tsx

dcrescimbeni · 2022-10-04T13:00:18Z

Nice! I've tested it and left some minor comments.

I think it works as intended now 👍

Also, it'll have conflicts with PR #369 , since we're both changing AddressInput, but shouldn't be anything significant.

About the pending tests: we should at least them to test the functionality of disabling the input if it has a default value and the unlock feature.

rulfo71 · 2022-10-23T15:40:25Z

"fix bug found where action index 2 on the contract for voting in dxvote is showing index 1 option when clicked"

this bug was caused because both functions had the same functionName "vote". I changed them to voteFor and voteAgainst and now it looks like this:

i'm not sure if it's the best ux for a non-technical user to see the function name and parameters but that's how it's done in other cases.

dcrescimbeni · 2022-11-08T11:38:31Z

I'm having the same issue as Filip.

Created a transfer proposal, then created a voting proposal with the transfer proposal data (proposal ID and guild address), and I'm having an infinite loop on the action and can't see it in the proposal page.

rulfo71 · 2022-11-09T19:46:07Z

hey @Filipv95 can you try it out now please?
it's working for me, here you have a video.

I created a proposal with just a transfer
i got the proposal id from the url
video, creating a proposal, adding the proposal id from 2)
Screencast from 09-11-22 16:31:25.webm
on the video you can see how i created the proposal. I ended it because my computer was being very slow but here you can see the proposal created:

please try editing it as well because i'm having a problem but i think it's just my computer resources

…rom-rich-contract-data

github-actions · 2022-11-10T12:55:46Z

✔️ Preview deployment is ready!

🔨 Explore the source changes: 070a360

😎 Browse the preview at one of these gateways:

github-actions · 2022-11-10T12:57:33Z

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeigyaealukmvhb2xp5mqo3cjw3emziququigkbe3o2dpxigkhdvffu.ipfs.cf-ipfs.com

github-actions · 2022-11-11T12:15:43Z

✔️ Preview deployment is ready!

🔨 Explore the source changes: dd4ef5b

😎 Browse the preview at one of these gateways:

github-actions · 2022-11-11T12:19:26Z

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeid4fyfbbacqy3jicpgt34uldvbyxqls5xsuxcp2nch746hyblleuq.ipfs.cf-ipfs.com

dcrescimbeni · 2022-11-11T13:06:15Z

I can make a transfer proposal and a voting proposal for it 👍

LGTM

0xvangrim · 2022-11-16T14:40:45Z

Hey Tomas! Sorry for not looking at this PR earlier. Have some comments on this issue:

Seems like we're missing some validation when the data length of the proposal ID is missing, for example. Should this be included in this PR or another issue? In my use case, I tried to add a wallet address instead of a proposal ID.

Some UI fixes needed for this (proposal ID is colliding with the buttons):

Personally, I think this looks a bit weird. As a user, I'd probably rather see "Vote YES with 1 REP". And then the vote decision should show YES or NO. But maybe it's a design decision that has been discussed already in which I rest my case.

0xvangrim

Code looks good ✅

0xvangrim · 2022-11-16T14:44:44Z

src/components/ActionsModal/types.ts

@@ -6,3 +6,8 @@ export interface ActionModalProps {
  onAddAction: (action: DecodedAction) => void;
  action?: DecodedAction;
 }
+
+export interface SelectedFunction {
+  functionName: string;


To keep it consistent. Should this be either functionName and functionTitle or name and title?

yes, this came from the richContracts but i agree with you, i'll change it :

github-actions · 2022-11-17T18:01:12Z

✔️ Preview deployment is ready!

🔨 Explore the source changes: bd49d4e

😎 Browse the preview at one of these gateways:

github-actions · 2022-11-17T18:02:22Z

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeiafimrb6p2ethd6y332tl4tmvc5cf4zrev5bx5jn2wzrrxfyrlf4u.ipfs.cf-ipfs.com

rulfo71 · 2022-11-18T11:07:51Z

Hey Tomas! Sorry for not looking at this PR earlier. Have some comments on this issue:

Seems like we're missing some validation when the data length of the proposal ID is missing, for example. Should this be included in this PR or another issue? In my use case, I tried to add a wallet address instead of a proposal ID.

Some UI fixes needed for this (proposal ID is colliding with the buttons):

Personally, I think this looks a bit weird. As a user, I'd probably rather see "Vote YES with 1 REP". And then the vote decision should show YES or NO. But maybe it's a design decision that has been discussed already in which I rest my case.

i'm validating now it is a bytes32 but i'm not sure if the text is the best for UX. open to suggestions:

github-actions · 2022-11-18T11:20:29Z

✔️ Preview deployment is ready!

🔨 Explore the source changes: 7a9b775

😎 Browse the preview at one of these gateways:

github-actions · 2022-11-18T11:22:16Z

✔️ Storybook deployment is ready!

😎 Browse Storybook: https://bafybeihpumckuuc224j4gb3iklllk4cohmxlwfklhxdsl2kvcyrcc5jb5m.ipfs.cf-ipfs.com

dcrescimbeni · 2022-11-18T17:53:50Z

Changes look good 👍

rossneilson · 2022-11-18T18:00:10Z

src/utils/bytes.ts

+const BYTES_32_LENGTH = 66;
+
+export function isBytes32(value: string) {
+  return value && value.length === BYTES_32_LENGTH;


We could check also if it starts with 0x

i thought about this but i'm not sure if it would be correct because of examples like this one i saw on richContracts:

this ones are component text so it isn't exactly the same but i thought it could happen.

just to be clear, if there is a possibility of having on richContracts a component "string" and type "bytes32" that it doesn't start with 0x we shouldn't add this condition.

Let me know what to think, no problem on adding it!

We are using the text input here because bytes 32 accepts numbers and text, so the bytes32 is just verification on the text. So we don't change anything about how we pass the data, just if we accept it. So since all bytes will have 0x at the start we can have 0x check as well as length (coming from the 32 number)
But we can merge this now and add it later

rossneilson

Amazing work Tomas!

rulfo71 added 3 commits September 26, 2022 19:32

Support default values from rich contract data

309b5b0

Merge remote-tracking branch 'upstream/develop' into default-values-f…

dea9b12

…rom-rich-contract-data

update snapshots

23c4d5d

fix case default value is 0.0 on numericalInput

376bf6e

update snapshots

5137289

rulfo71 changed the title ~~Default values from rich contract data~~ Feat #349 Default values from rich contract data Sep 28, 2022

rulfo71 changed the title ~~Feat #349 Default values from rich contract data~~ Feat #297 Default values from rich contract data Sep 28, 2022

dcrescimbeni reviewed Sep 28, 2022

View reviewed changes

rulfo71 changed the title ~~Feat #297 Default values from rich contract data~~ Feat #297 Support default values from rich contract data Sep 29, 2022

Merge remote-tracking branch 'upstream/develop' into default-values-f…

0cb3781

…rom-rich-contract-data

rossneilson reviewed Sep 30, 2022

View reviewed changes

show unlock icon if param has defaultValue

b5d950c

dcrescimbeni reviewed Oct 4, 2022

View reviewed changes

src/components/primitives/Forms/AddressInput.tsx Outdated Show resolved Hide resolved

src/components/primitives/Forms/IconRight.tsx Outdated Show resolved Hide resolved

rulfo71 added 2 commits October 10, 2022 17:14

fix name setDisabledState

e8be4e3

remove unlock icon when clicked

efe036d

rulfo71 added 3 commits October 23, 2022 10:42

fix voting functions on actions had the same name

6dc20c7

added tests

14a4a22

change tests on addressInput to IconRight

559949c

fix network address on richContract json was repeated

03427cf

Merge remote-tracking branch 'upstream/develop' into default-values-f…

8e1f86b

…rom-rich-contract-data

fix on getting richContracts from ens

631f11d

rulfo71 requested review from rossneilson, MiltonTulli, mprasanjith and 0xvangrim November 11, 2022 18:02

0xvangrim reviewed Nov 16, 2022

View reviewed changes

rulfo71 added 2 commits November 17, 2022 10:18

add validation for bytes32

5f2695c

shorten proposalId on action

a2890ac

rulfo71 requested review from 0xvangrim and dcrescimbeni November 17, 2022 20:33

change name of variable for consistency

eb0d868

rossneilson reviewed Nov 18, 2022

View reviewed changes

rossneilson approved these changes Nov 18, 2022

View reviewed changes

rulfo71 merged commit c6500be into DXgovernance:develop Nov 21, 2022

Feat #297 Support default values from rich contract data #349

Feat #297 Support default values from rich contract data #349

Conversation

rulfo71 commented Sep 26, 2022

Description

Type of change

How Has This Been Tested?

Checklist:

github-actions bot commented Sep 26, 2022

github-actions bot commented Sep 26, 2022

github-actions bot commented Sep 27, 2022

github-actions bot commented Sep 27, 2022

github-actions bot commented Sep 27, 2022

github-actions bot commented Sep 27, 2022

dcrescimbeni commented Sep 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rulfo71 Sep 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossneilson commented Sep 30, 2022

github-actions bot commented Oct 3, 2022

rulfo71 commented Oct 3, 2022

dcrescimbeni commented Oct 4, 2022 • edited

rulfo71 commented Oct 23, 2022

dcrescimbeni commented Nov 8, 2022

rulfo71 commented Nov 9, 2022 • edited

github-actions bot commented Nov 10, 2022

github-actions bot commented Nov 10, 2022

github-actions bot commented Nov 11, 2022

github-actions bot commented Nov 11, 2022

dcrescimbeni commented Nov 11, 2022

0xvangrim commented Nov 16, 2022

0xvangrim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 17, 2022

github-actions bot commented Nov 17, 2022

rulfo71 commented Nov 18, 2022

github-actions bot commented Nov 18, 2022

github-actions bot commented Nov 18, 2022

dcrescimbeni commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossneilson left a comment

Choose a reason for hiding this comment

dcrescimbeni commented Sep 27, 2022 •

edited

rulfo71 Sep 28, 2022 •

edited

dcrescimbeni commented Oct 4, 2022 •

edited

rulfo71 commented Nov 9, 2022 •

edited